fix: make prepared FTS INDEX_ONLY scans segment-scoped - #69
Conversation
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Removing the scanner-wide fast-search mode is the right correction, but the replacement fragment restriction is still too broad: it makes prepared INDEX_ONLY FTS scan every selected indexed row before the index query and turns valid empty segment assignments into internal errors.
A viable revision should suppress only the FTS flat-fallback arm while retaining ordinary scalar-filter fallback and explicit _rowid semantics, and it should short-circuit zero-current-coverage segment assignments to an empty result.
| .is_ok_and(|fragment_id| selected_fragment_ids.contains(&fragment_id)) | ||
| }) | ||
| .map(|fragment| fragment.metadata().clone()) | ||
| .collect(); |
There was a problem hiding this comment.
selected_fragments can legitimately be empty when a committed FTS segment has zero or stale-only current coverage. With an unindexed tail, passing that empty set to Lance makes plan_match_query choose the all-targets-unindexed flat path, so this wrapper finds zero MatchQueryExec nodes and returns the internal expected exactly one MatchQueryExec ... replaced 0 error instead of an empty shard. Zero-fragment FTS segments are explicitly supported upstream, so a distributed planner can assign one.
Reproducer run on this head
I added a disposable integration test that: (1) builds and commits two inverted-index segments named segmented_fts, one via .fragments(Vec::new()) and one for fragment 0; (2) appends an unindexed fragment; (3) prepares an INDEX_ONLY context; (4) selects only the empty segment UUID; and (5) asserts lance_scanner_to_arrow_stream(...) == 0 and zero output rows.
cargo test repro_prepared_fts_empty_selected_segment -- --nocaptureThe assertion observed -1 with expected exactly one MatchQueryExec in prepared FTS plan, replaced 0 at src/scanner.rs:370; the expected result is a successful empty stream.
Please short-circuit empty effective coverage to an empty result before requiring a replacement, and keep a regression with an empty/stale-only selected segment plus an unindexed live fragment.
There was a problem hiding this comment.
Fixed in 6f0fae4: selected segments with no current fragment coverage are now rewritten to schema-preserving EmptyExec branches, and the empty-segment regression verifies a successful zero-row stream.
| }) | ||
| .map(|fragment| fragment.metadata().clone()) | ||
| .collect(); | ||
| scanner.with_fragments(selected_fragments); |
There was a problem hiding this comment.
with_fragments is scanner-wide, so this trades the old global side effects for a new linear scan. In the pinned Lance planner, any explicit fragment set changes an otherwise unfiltered FTS prefilter from None to FilteredRowIds; it scans every selected fragment's row IDs, materializes an allow-list, and MatchQueryExec waits for that list before searching the inverted index. On large indexed fragments this defeats INDEX_ONLY query scaling and can consume memory proportional to all selected rows.
Reproducer run on this head
I instrumented the existing test_prepare_fts_query_index_only_allows_unindexed_fragment by attaching its CapturedScanStatistics callback before stream creation, consuming the stream, and printing the dynamic metrics.
cargo test test_prepare_fts_query_index_only_allows_unindexed_fragment -- --nocaptureAt the base revision the one-hit query reported index metrics only. On this head the same test additionally reported rows_scanned=5, fragments_scanned=1, and ranges_scanned=1, exactly the full indexed fragment. The planner source shows this work grows with the selected fragment rows, not the hit count.
Please suppress only the FTS flat-fallback plan branch (or add/use a targeted upstream FTS coverage control) instead of applying a scanner fragment filter, so scalar-filter fallback remains enabled without a row-ID pre-scan.
There was a problem hiding this comment.
Fixed in 6f0fae4: the scanner-wide fragment restriction was removed. The plan rewrite now drops only FlatMatchQueryExec, and the unfiltered indexed branch retains PreFilterSource::None; the plan-shape and unindexed-tail regressions pass.
lance-gatekeeper
Bot
added
the
K-changes
lance-gatekeeper
Bot
removed
the
K-changes
zhangstar333
changed the title
There was a problem hiding this comment.
✅ Gate recommendation: approve.
Both earlier findings are fixed. Prepared INDEX_ONLY scans now bind only the selected index segments, remove the unindexed flat-search branch without introducing a fragment row-ID scan, and return schema-correct empty results when selected coverage is empty.
lance-gatekeeper
Bot
added
the
K-approved
|
@jja725 PTAL |
### What problem does this PR solve? Issue Number: None Related PR: lance-format/lance-c#69 Problem Summary: Lance-C v0.1.8 now contains the upstream changes previously carried by the v0.1.7 PR apache#64 and runtime-filter patches. Upgrade the bundled source archive and checksum, remove both obsolete v0.1.7 patches, and retain only the current two-commit snapshot of upstream PR apache#69 for prepared FTS INDEX_ONLY plan scoping and explicit _rowid behavior. ### Release note Upgrade the bundled Lance-C dependency to v0.1.8 and apply upstream PR apache#69. ### Check List (For Author) - Test: Manual test - Verified the official archive MD5, applied PR apache#69 to a clean v0.1.8 archive with zero fuzz, compared every tracked file byte-for-byte with PR head 6f0fae4, checked shell syntax, Rustfmt, stale references, and upstream CI status - Behavior changed: Yes. Prepared FTS INDEX_ONLY scans use the PR apache#69 plan rewrite without scanner-wide fast_search - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: lance-format/lance-c#69 Problem Summary: Lance-C v0.1.8 now contains the upstream changes previously carried by the v0.1.7 PR apache#64 and runtime-filter patches. Upgrade the bundled source archive and checksum, remove both obsolete v0.1.7 patches, and retain only the current two-commit snapshot of upstream PR apache#69 for prepared FTS INDEX_ONLY plan scoping and explicit _rowid behavior. ### Release note Upgrade the bundled Lance-C dependency to v0.1.8 and apply upstream PR apache#69. ### Check List (For Author) - Test: Manual test - Verified the official archive MD5, applied PR apache#69 to a clean v0.1.8 archive with zero fuzz, compared every tracked file byte-for-byte with PR head 6f0fae4, checked shell syntax, Rustfmt, stale references, and upstream CI status - Behavior changed: Yes. Prepared FTS INDEX_ONLY scans use the PR apache#69 plan rewrite without scanner-wide fast_search - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: lance-format/lance-c#69 Problem Summary: Lance-C v0.1.8 now contains the upstream changes previously carried by the v0.1.7 PR apache#64 and runtime-filter patches. Upgrade the bundled source archive and checksum, remove both obsolete v0.1.7 patches, and retain only the current two-commit snapshot of upstream PR apache#69 for prepared FTS INDEX_ONLY plan scoping and explicit _rowid behavior. ### Release note Upgrade the bundled Lance-C dependency to v0.1.8 and apply upstream PR apache#69. ### Check List (For Author) - Test: Manual test - Verified the official archive MD5, applied PR apache#69 to a clean v0.1.8 archive with zero fuzz, compared every tracked file byte-for-byte with PR head 6f0fae4, checked shell syntax, Rustfmt, stale references, and upstream CI status - Behavior changed: Yes. Prepared FTS INDEX_ONLY scans use the PR apache#69 plan rewrite without scanner-wide fast_search - Does this need documentation: No
1 participant
MatchQueryExecdirectly to the index segments selected by the external planner.FlatMatchQueryExecbranch forINDEX_ONLYscans without enabling scanner-widefast_searchorwith_fragments._rowidonly when explicitly requested.